feat(acp/desktop): classify turn failures and persist them per agent - #2240
feat(acp/desktop): classify turn failures and persist them per agent#2240Orland00 wants to merge 5 commits into
Conversation
Add a stable, machine-readable error_class discriminant (timeout, transport, agent_error, protocol, exited, cancelled, panic, error) to the turn_error and agent_panic observer payloads, additive alongside the existing outcome/error/code fields. Extracted into a pure classify_turn_failure(&PromptOutcome) so the mapping is unit-testable independent of the emit call sites, and it mirrors the existing outcome_label/is_transport_error groupings rather than inventing a new taxonomy. Signed-off-by: Orlando Lopez <orlando.lf19@gmail.com>
…discarding them activeAgentTurnsStore previously treated turn_error/agent_panic the same as turn_completed: endTurn() ran and the outcome/error/code payload was discarded, so the "Working in #channel" badge just vanished with no trace of the failure. Add a per-agent TurnFailure record (outcome, error, code, errorClass, timestamp) captured from the turn_error/agent_panic payload — including the new additive error_class field from the backend commit — and exposed via getLastTurnFailureForAgent / useLastTurnFailure. It is cleared only on a subsequent turn_completed (a successful completion is the "agent is healthy again" signal; a mere turn_started is not, so the badge doesn't flicker off and back on). Both new branches sit behind the same per-agent watermark gate as before, so the existing stale-replay guarantees (a replayed turn_error/agent_panic with a null turnId must not resurrect or re-trigger effects on a live/completed turn) are unchanged — verified with a new regression test. The failure map is also folded into the community-switch save/restore snapshot alongside the other four maps. Wired into ManagedAgentRow's existing StatusBlock, next to the process-exit-sourced `agent.lastError` surface, rendered through friendlyTurnErrorCopy so JSON-RPC codes still get friendly copy. Signed-off-by: Orlando Lopez <orlando.lf19@gmail.com>
2128600 to
dc4869b
Compare
Signed-off-by: Orlando Lopez <orlando.lf19@gmail.com>
Signed-off-by: Orlando Lopez <orlando.lf19@gmail.com>
wpfleger96
left a comment
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Agent-authored review (two independent passes; findings converged, so treat the blocker as high-confidence).
Really solid contribution overall — the watermark-gate analysis is correct, errorClass being nullable for mixed-version fleets is the right call, the classification fn is pure with every branch tested, and the stale-replay regression test is exactly the right non-happy-path. The failure map also correctly joins reset + community-switch save/restore.
I re-ran the gates locally at ce7f1228: cargo test -p buzz-acp 580/0, clippy -D warnings clean, fmt --check clean, desktop typecheck/lint/test (3339/0)/build all green. The checks in the PR description are accurate.
Requesting changes for one correctness issue in the clear path (inline below): turn_completed is emitted on every exit path by TurnCompletionGuard (crates/buzz-acp/src/pool.rs:3218-3258), not just success, so a cancelled turn wipes a persisted failure and never re-records it. Details and a minimal suggested fix on the store's turn_completed branch. The other inline comments are non-blocking nits — take or leave them.
| * from a harness build that predates the field. */ | ||
| errorClass: string | null; | ||
| /** Agent-host clock ms when the failure was recorded (parsed from the event). */ | ||
| timestamp: number; |
There was a problem hiding this comment.
Nit: timestamp is stored and asserted in tests but never rendered anywhere — dead field today. Fine if a tooltip/relative-time display is coming; otherwise it's removable.
There was a problem hiding this comment.
🤖 Still open at the latest head — timestamp is stored and asserted but nothing renders it. Could you either wire it into the UI (relative time on the error line would do) or drop the field? Fine either way, just don't want a dead field shipping.
| data-testid="managed-agent-last-turn-error" | ||
| > | ||
| Last turn error ( | ||
| {lastTurnFailure.errorClass ?? lastTurnFailure.outcome}):{" "} |
There was a problem hiding this comment.
Nit: this renders the raw machine token in user-facing copy (“Last turn error (agent_error): …”). A tiny humanizing map — or the prettify helper already in agentSessionUtils — would read better. Product call, non-blocking.
There was a problem hiding this comment.
🤖 Still showing the raw machine token at the latest head ("Last turn error (agent_error): …"). Could you run it through a small humanizing map (or the prettify helper in agentSessionUtils) before this merges? The classes are a closed set so it's a ~8-line map.
| {friendlyError.copy} | ||
| </p> | ||
| ) : null} | ||
| {lastTurnFailure ? ( |
There was a problem hiding this comment.
Nit: for error_class == "exited" this line will stack with the process-exit friendlyError line just above in the same StatusBlock — two error lines describing one crash. Consider suppressing the turn-failure line when a process-level error is already shown.
There was a problem hiding this comment.
🤖 Still reproducible at the latest head — an exited turn failure renders alongside the process-exit friendlyError line in the same StatusBlock, so one crash reads as two errors. Could you suppress the turn-failure line when a process-level error is already shown?
…pleted Review follow-up for block#1659: TurnCompletionGuard emits `turn_completed` on every exit path (success, error, timeout, cancel, panic), so the desktop store cleared a persisted failure badge on a *cancelled* turn too — erasing a prior genuine failure that a cancel proves nothing about. - buzz-acp: tag the `turn_completed` payload with a coarse `outcome`, set to "ok" only on the `PromptOutcome::Ok` send paths (via a shared success flag on the completion guard) and "incomplete" on every other exit. - desktop store: clear the failure only when `outcome` is "ok" or absent (absent = a harness build predating the field, so mixed-version behavior is unchanged). A cancelled/failed completion no longer clears the badge. - add a regression test: failure -> cancelled turn_completed -> failure remains; an "ok" completion still clears it. - classify_turn_failure: document the unreachable `Ok(_)` defensive default. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Orlando Lopez <orlando.lf19@gmail.com>
|
Thanks for the detailed review @wpfleger96 — pushed 3cc3dae addressing it. Blocker (
ManagedAgentRow nits: left the two UI lines as a follow-up since they're product calls — happy to push both if you'd like: (1) suppress the turn-failure line when a process-level error is already shown for Gates green locally at 3cc3dae: |
2 participants
Closes #1659.
Problem
Failed turns still burn tokens; when timeouts, transport flakes, auth expiry and panics all collapse into one vanishing "Turn error" badge, that spend is impossible to attribute or reduce. As discussed on the issue, the backend already derives a granular
outcome_label— the collapse happens in the desktop store, which handlesturn_error/agent_panicin the same fall-through asturn_completedand discards the entire error payload.Implementation
Two commits (split-able into separate PRs on request):
feat(acp)— additiveerror_classfield on theturn_errorandagent_panicpayloads: a stable machine discriminant (timeout/transport/agent_error/protocol/exited/cancelled/panic) derived in a pureclassify_turn_failure(&PromptOutcome)fn from the same data that drivesoutcome_labeland the existingis_transport_errorgrouping. Existing payload fields untouched; old frontends unaffected. No central schema doc exists for observer event kinds (grepped docs/, AGENTS.md, NOSTR.md), so the fn's doc comment is the documentation site, matching the existing convention.feat(desktop)—activeAgentTurnsStorenow splits the terminal-event fall-through:turn_completedclears a new per-agentlastFailureByAgententry;turn_error/agent_panicpersist{outcome, error, code, error_class, timestamp}. Both remain inside the pre-existing per-agent watermark gate, so the stale-replay guarantee (replayedturn_errorwith nullturnIdmust not resurrect state) is preserved — covered by a new regression test. The persisted failure renders inManagedAgentRow's existingStatusBlockthrough the existingfriendlyTurnErrorCopy, next to the process-exitlastErrorline — no new UI surface. Clearing happens on the next successful completion, not on turn start: a turn starting proves nothing about health and would make the badge flicker.Design notes:
errorClassis nullable on the frontend so a mixed-version fleet (older harness without the field) degrades gracefully tooutcome.resetActiveAgentTurnsStoreand the community-switch save/restore snapshot like the other four maps.Testing
classify_turn_failurebranch + payload assertions in the existingerror_outcome_emission_tests;cargo test -p buzz-acp580 passed,cargo clippy -p buzz-acp --all-targets -- -D warningsclean,cargo fmt --all -- --checkclean,cargo check --workspaceclean.pnpm -C desktop test3339 passed / 0 failed;typecheckandlint(biome) clean.Manual test: trigger any turn failure (e.g. kill the agent process mid-turn) → the agent row shows "Last turn error (exited): …" persistently; run a successful turn → the line clears.
Deferred
ManagedAgentRow's newmanaged-agent-last-turn-errornode (repo has no component-level tests for this row today) — happy to add a Playwright assertion if you want one.